-
Notifications
You must be signed in to change notification settings - Fork 12.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Check lstat before calling realpath #41288
Conversation
The only thing we're using realpath for is to resolve symlinks and most files and directories are not symlinks. This change skips the expensive realpath call for non-symlinks.
@typescript-bot perf test this |
Heya @amcasey, I've started to run the perf test suite on this PR at 56f9d5e. You can monitor the build here. Update: The results are in! |
I don't expect the perf test to show an improvement since the benefit to tsc seems to be negligible (the expense in tsserver seems to come primarily from |
@amcasey Here they are:Comparison Report - master..41288
System
Hosts
Scenarios
|
Found the catch - this will do the wrong thing for a non-symlink file in a symlinked directory. |
To be correct, this would need to lstat recursively on the path and I don't see how that could be faster than what we have now. |
The only thing we're using realpath for is to resolve symlinks and most files and directories are not symlinks. This change skips the expensive realpath call for non-symlinks.
Q: Shouldn't we add lstat to the System interface?
A: Strictly speaking, we probably should, but the behavior of our realpath isn't actually documented anywhere, so we're not really locked in, and we want this at all call sites, so we'd have to add a layer of abstraction anyway.
Q: What if it is a symlink? Won't it be slower?
A: I believe (based on previous forays in the node/libuv codebase), but haven't proven, that libuv (and probably the OS) caches lstat results so the "real" lstat call in realpath should be basically free, for a negligible net increase.
Q: How was this validated?
A: This was inspired by my profiling of project loading (specifically
createProgram
) in one of the Office codebases. In that code, the project loading time is 6% faster on Windows and 20% faster on Linux (we call realpath as part of many of our existence checks and I conjecture that the lack of recursive file watching on Linux requires more of these). I also confirmed thatxstate-analytics
, which is part of a lerna monorepo and pulls in dependencies via symlinks in node_modules, was about 5% faster on Windows.